-
Notifications
You must be signed in to change notification settings - Fork 8.3k
eth: stm32h7: use SRAM3 / MPU configuration #30403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you very much for this very useful PR. Now my ideas on it:
2.@reloZid Very elegant fix from you here! But we can't just change linker scripts every time we need specific memory regions configuration. I think here we need memory reallocation and MPU configuration possibilities in DT @gmarull @erwango @galak Now we seems to have proper solution for solide bug fix with future improvents for the whole Zephyr project. What do you think? Especially, what do you think about MPU configuration and data regions definition in DT? |
|
Thanks for this draft, will be useful to trigger discussions on how to find a proper fix for this problem and also future similar requirements. I see some other projects have a sort of MPU API, e.g. https://github.com/u-boot/u-boot/blob/master/arch/arm/mach-stm32/soc.c In Zephyr I think something similar can be done if using SRAM3 could be configured entirely for that purpose at soc level, and just provide DT definitions for sram3 together with linker scripts facilities so that Ethernet buffers can easily be placed there. I'm not sure if that's the best approach, though. I'm not really familiar with Zephyr MPU implementation, it would be good to have input from maintainers @ioannisg @galak @MaureenHelm @stephanosio @carlocaione |
|
We need a possibility to configure MPU and SRAM sections over DT or KConfig for Zephyr. And then we need a possibility to use configured SRAM sections in our code. |
|
dev-review (Dec 3, 2020): Suggestion to utilize CONFIG_NOCACHE_MEMORY and look at adding one more section for the SRAM specific case in arch/arm/core/aarch32/cortex_m/mpu/arm_core_mpu.c. @ioannisg would like to see over time cleaning up of the linker script and mpu tables to move into SoC specific code. |
I would prefer not to add half-solutions. We can better define amount of work and split it here, providing proper and solid solution. My idea is to add This needs changes in DT parser or at least an additional preprocessing step before original DT. In this preprocessing step west/zephyr specifig artifacts (like Any ideas to my proposal?
|
KozhinovAlexander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind the changes is very good. But we need more scalable solution.
@Nukersson, this point was discussed during dev-review meeting. Of course this proposal lacks scalability. Though w/o alternative proposal we find it acceptable for now with few tweaks. |
Okay. I understand it. But then we need to plan proper solution within the roadmap for next year or two. |
ioannisg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks acceptable to me, but I would insist on trying using DT for RAM region definition and use the DT macros in the linker script and other locations. And use the SOC_SPECIFIC MPU region definition option and define the MPU region at the SoC level (not in the arm mpu driver)
Future:
I would really like to clean this up; in particular, the soc could add hooks to the mpu driver at run time, to define static MPU partitions that are SoC-specific, not feature-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be an #ifdef directive based on DT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
soc/arm/st_stm32/stm32h7/linker.ld
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be needed, imho, if we use device tree for getting the RAM3 address and size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine. I wonder if you can add the declaration for the ARMv8-M architecture as well.
Optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed, therefore removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: see if you could have it in a single line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed, therefore removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is
- SoC specific, not feature specific, and,
- outside the image boundaries (image_ram_start, image_ram_end)
So I think we should try to define this MPU region at boot. See soc/arm/common/arm_mpu_regions.c. As discussed in the dev-review meeting there is a Kconfig option that allows to override the default SoC MPU region definition, and add a SoC-specific one that includes this region as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this hint! Added SoC specific MPU region definitions.
drivers/ethernet/eth_stm32_hal.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there still be a Kconfig option to enable this for the driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erwango ^^
dts/arm/st/h7/stm32h747.dtsi
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned this with the H745 device tree which should have the same memory architecture. Anyone can confirm this is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a check to #30530.
I'm reworking that part a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned this with the H745 device tree which should have the same memory architecture. Anyone can confirm this is correct?
Yes. They have same memory architecture. They also have same ReferenceManual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied this from soc/arm/common/cortex_m/arm_mpu_mem_cfg.h. Can we share this somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erwango ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible to include soc/arm/common/cortex_m/arm_mpu_mem_cfg.h directly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included it directly. Wasn't sure because it seems only possible using a relative path:
#include "../../common/cortex_m/arm_mpu_mem_cfg.h"
|
@reloZid it might be interested for you to rebase on top of latest master, following the merge of #30530. |
|
@reloZid would you mind rebasing ? |
Define SRAM1 to SRAM4 memory areas according to the physical memory organization of the chips. Signed-off-by: Mario Jaun <mario.jaun@gmail.com>
Add a linker section for SRAM3/4 if it is enabled in the device tree. Signed-off-by: Mario Jaun <mario.jaun@gmail.com>
Add a define for non cachaeable RAM for MPU region definitions. Signed-off-by: Mario Jaun <mario.jaun@gmail.com>
Fixes zephyrproject-rtos#29915. Implements the memory layout and MPU configuration for Ethernet buffers for STM32H7 controllers as recommended by ST. 16 KB of SRAM3 are are reserved for this. The first 256 B are for the RX/TX descriptors and configured as strongly ordered, shareable memory. The rest is for RX/TX buffers and configured as non cacheable memory. This configuration is automatically applied for H7 chips if the SRAM3 memory is enabled in the device tree. Signed-off-by: Mario Jaun <mario.jaun@gmail.com>
|
@erwango Sorry for the late reaction. I rebased the changes. |
|
@ioannisg can you have a new look ? |
PR zephyrproject-rtos#30403 implemented nocache regions for ethernet DMA buffers in sram3 to fix issue zephyrproject-rtos#29915. Unfortunately, some STM32H7 variants do not have any sram3 so they still suffer from zephyrproject-rtos#29915. All H7 variants have sram2 though, so use that for targets without sram3. Signed-off-by: Björn Stenberg <bjorn@haxx.se>
PR #30403 implemented nocache regions for ethernet DMA buffers in sram3 to fix issue #29915. Unfortunately, some STM32H7 variants do not have any sram3 so they still suffer from #29915. All H7 variants have sram2 though, so use that for targets without sram3. Signed-off-by: Björn Stenberg <bjorn@haxx.se>

Fixes: #29915
Changes are based on these recommendations.
Notes: